[TM] Add spec for AlertManager#24906
Conversation
facebook-github-bot
added
the
CLA Signed
analysis-bot
left a comment
analysis-bot
left a comment
There was a problem hiding this comment.
Code analysis results:
eslintfound some issues. Runyarn lint --fixto automatically fix problems.
react-native-bot
added
API: Alert
Type: Enhancement
RSNara
left a comment
RSNara
left a comment
There was a problem hiding this comment.
Thanks!
But unfortunately, our current Codegen system doesn't support enums or unions. Could you replace them with strings?
There was a problem hiding this comment.
Unfortunately, our Codegen doesn't support unions. 😔
There was a problem hiding this comment.
It also doesn't support enums. 😔
|
@RSNara Thanks for reviewing. 👍 |
sasurau4
force-pushed
the
spec-alertmanager
branch
from
fbceb18 to
2b5b22f
Compare
|
@uqmessias Thanks for reviewing 👍 |
| ) => void; | ||
| } | ||
|
|
||
| export default TurboModuleRegistry.getEnforcing<Spec>('AlertManager'); |
There was a problem hiding this comment.
Since, this is an iOS only feature, please export it conditionally.
|
It's almost there, just left a few comments |
| destructive: string, | ||
| }>; | ||
| /* 'default' | 'cancel' | 'destructive' */ | ||
| export type AlertButtonStyle = string; |
There was a problem hiding this comment.
Good job!
But it would be better to keep this inside the spec file, because there is where we're defining types. We're could still exporting it here too or updating all call-sites that uses these types from here to the spec path.
What do you think about it?
There was a problem hiding this comment.
I agree with keeping types inside the spec file. I'll move types to spec file 👍
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
analysis-bot
left a comment
analysis-bot
left a comment
There was a problem hiding this comment.
Code analysis results:
eslintfound some issues. Runyarn lint --fixto automatically fix problems.
|
@uqmessias Thanks for comments! I worked on it! Plz review 🙏 |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| type AlertType, | ||
| type AlertButtonStyle, | ||
| } from './NativeAlertManager'; | ||
| import {type Buttons, type Options, type AlertType} from './NativeAlertManager'; |
There was a problem hiding this comment.
This is a minor and no need to update it, but you also can use
import type { Buttons, Options, AlertType } from './NativeAlertManager';
|
We still need @RSNara's approval |
|
This pull request was successfully merged by @sasurau4 in 122cc8b. When will my fix make it into a release? | Upcoming Releases |
Summary: Part of facebook#24875 ## Changelog [General] [Added] - Add TurboModule spec for AlertManager Pull Request resolved: facebook#24906 Reviewed By: lunaleaps Differential Revision: D15471065 Pulled By: fkgozali fbshipit-source-id: bb22e6454b1f748987f3a8cd957bfd4e027493a5
8 participants
Summary
Part of #24875
Changelog
[General] [Added] - Add TurboModule spec for AlertManager
Test Plan
yarn flow